Skip to content

Make Evapotranspiration differentiable and vectorized#78

Merged
SCiarella merged 47 commits intomainfrom
evapo
Feb 24, 2026
Merged

Make Evapotranspiration differentiable and vectorized#78
SCiarella merged 47 commits intomainfrom
evapo

Conversation

@SCiarella
Copy link
Collaborator

Closes #44.

@SCiarella SCiarella marked this pull request as ready for review February 17, 2026 11:10
Copy link
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SCiarella thanks, looks very good, great to see how you cleaned up the classes by defining base classes 👍 I left some suggestions mostly about docstring and missing in-line comments. My main concerns are related to these:

  • implementing sSTE method with sigmoid insted of the cubic smoothstep,
  • adding more parameters to tests of gradients
  • implementing finalize method
  • implementing dvs mask

please see my comments for details. If something isnot clear, let me know and we can discuss it offline.

Copy link
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SCiarella thanks, looks very good, great to see how you cleaned up the classes by defining base classes 👍 I left some suggestions mostly about docstring and missing in-line comments. My main concerns are related to these:

  • implementing STE method with sigmoid insted of the cubic smoothstep,
  • adding more parameters to tests of gradients
  • implementing finalize method
  • implementing dvs mask

please see my comments for details. If something isnot clear, let me know and we can discuss it offline.

SCiarella and others added 19 commits February 23, 2026 09:58
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
SCiarella and others added 14 commits February 23, 2026 11:15
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
@SCiarella
Copy link
Collaborator Author

Thanks a lot @SarahAlidoost for the careful review!

I have implemented all your suggestions, in particular:

  • we now use STE+sigmoid to differentiate the step function (same as leaf)
  • we test all the parameters combination for the gradient
  • implemented a finalize method like in the pcse version
  • we use dvs_mask like in the other modules

I have addressed and closed all the review comments that you left, except for a couple of minor comments that I did not fully understand, so please update that if they are still relevant after the revision.

Copy link
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SCiarella Awesome! thanks for addressing the comments. Looks very good! 👍 only some suggestions on the docstring. I noticed some units are not the same as pcse code, see my suggestions. After applying those, let's merge this PR for now.

P.S. I'm not sure why in pcse there are different types for one variable for example SCr and S for the variable CFET. We can check this in our update meeting.

SCiarella and others added 5 commits February 24, 2026 09:24
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
@SCiarella SCiarella merged commit f55995e into main Feb 24, 2026
10 checks passed
@SCiarella SCiarella deleted the evapo branch February 24, 2026 08:27
@sonarqubecloud
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task]: Make Evapotranspiration differentiable and efficient

2 participants